Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIO-8645: day component required validation error not correct #5687

Conversation

ZenMasterJacob20011
Copy link
Contributor

Link to Jira Ticket

https://formio.atlassian.net/browse/FIO-8645

Description

What changed?

Previously, formio.js did not have the requiredDayField translation key in the en.js file which caused the required validation to not display correctly. This PR replaces this behavior by adding requiredDayField translation key to the translation file

Dependencies

N/A

How has this PR been tested?

I added automated tests and manually tested

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (if applicable)
  • My changes generate no new warnings
  • My changes include tests that prove my fix is effective (or that my feature works as intended)
  • New and existing unit/integration tests pass locally with my changes
  • Any dependent changes have corresponding PRs that are listed above

Copy link
Member

@travist travist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we also fix this by just changing "requiredDayField" to just "required" and not introduce a new translation that is exactly the same as "required"?

Copy link
Contributor Author

@ZenMasterJacob20011 ZenMasterJacob20011 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In @formio/core/src/process/validation/rules there is a file called validateRequiredDay.ts. This seems to be added in because you can set the required validation check on the day, month, and year inputs individually. Right now we have three translation keys for the day component and that is requiredDayField, requiredMonthField, and requiredYearField. I can either ...

  • change requiredDayField, requiredMonthField, and requiredYearField to just required
  • delete the validateRequiredDay.ts file and pass the responsibility back to some function inside the validateRequired.ts file. This would require a check to see if the component is a day component and some extra validation checking due to the fact that you can set required validations for day, month, and year individually.

I've also just noticed that even if you have required validation on for all of them, the validator will only throw an error for one of them in the order of day, month, and year. So if you have all of them set to required only the day will be marked as a required error

@ZenMasterJacob20011
Copy link
Contributor Author

Talked to Brenden this morning in dev support, he is ok with having a translation key for the day component required validations for day, month, and year.

The

"I've also just noticed that even if you have required validation on for all of them, the validator will only throw an error for one of them in the order of day, month, and year. So if you have all of them set to required only the day will be marked as a required error"

problem will need another separate ticket to solve it. Changing the type of the validateRequiredDaySync to a function that returns a list of FieldErrors was starting to become challenging for me and required very many changes to the types of RuleFn, RuleFnSync, ValidationRuleInfo, and more. Not only that but changing all these types to support FieldError arrays would require array type checking on variables of type ValidationRuleInfo because you would need to check whether you are dealing with a FieldError or a FieldError[]

Copy link
Contributor

@brendanbond brendanbond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a comment

@ZenMasterJacob20011
Copy link
Contributor Author

Added core tests and translations
formio/core#119

Copy link
Contributor Author

@ZenMasterJacob20011 ZenMasterJacob20011 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added core tests and translations
formio/core#119

Copy link
Contributor Author

@ZenMasterJacob20011 ZenMasterJacob20011 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed

@brendanbond brendanbond merged commit d5246a5 into master Jul 17, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants